Closed Bug 1599173 Opened 6 years ago Closed 6 years ago

heap-buffer-overflow in [@ IsCSSWordSpacingSpace]

Categories

(Core :: Layout: Text and Fonts, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 + verified

People

(Reporter: tsmith, Assigned: heycam)

References

(Blocks 1 open bug, Regression)

Details

(5 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file testcase.html

Reduced with m-c:
BuildID=20191125161209
SourceStamp=b4755981c1382cb88fed4e4fcff3ba73779b2080

==120354==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020001631d4 at pc 0x7f81246afb98 bp 0x7ffe0ccf46a0 sp 0x7ffe0ccf4698
READ of size 1 at 0x6020001631d4 thread T0 (file:// Content)
    #0 0x7f81246afb97 in nsTextFragment::CharAt(int) const src/dom/base/nsTextFragment.h:217:54
    #1 0x7f812a1f59dc in IsCSSWordSpacingSpace src/layout/generic/nsTextFrame.cpp:766:24
    #2 0x7f812a1f59dc in nsTextFrame::PropertyProvider::GetSpacingInternal(gfxTextRun::Range, gfxFont::Spacing*, bool) const src/layout/generic/nsTextFrame.cpp:3452:13
    #3 0x7f81242ad30f in GetAdjustedSpacing src/gfx/thebes/gfxTextRun.cpp:351:14
    #4 0x7f81242ad30f in gfxTextRun::GetAdvanceWidth(gfxTextRun::Range, gfxTextRun::PropertyProvider*, gfxFont::Spacing*) const src/gfx/thebes/gfxTextRun.cpp:1162:7
    #5 0x7f812a3a675b in SVGTextFrame::DetermineCharPositions(nsTArray<nsPoint>&) src/layout/svg/SVGTextFrame.cpp:4425:22
    #6 0x7f812a3abec9 in SVGTextFrame::DoGlyphPositioning() src/layout/svg/SVGTextFrame.cpp:4803:3
    #7 0x7f812a396661 in UpdateGlyphPositioning src/layout/svg/SVGTextFrame.cpp:5002:5
    #8 0x7f812a396661 in SVGTextFrame::ReflowSVG() src/layout/svg/SVGTextFrame.cpp:3428:3
    #9 0x7f812a39749f in nsSVGDisplayContainerFrame::ReflowSVG() src/layout/svg/nsSVGContainerFrame.cpp:319:17
    #10 0x7f812a407503 in nsSVGOuterSVGFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/svg/nsSVGOuterSVGFrame.cpp:459:14
    #11 0x7f8129cc8a8f in mozilla::PresShell::DoReflow(nsIFrame*, bool, mozilla::OverflowChangedTracker*) src/layout/base/PresShell.cpp:9179:11
    #12 0x7f8129ce1547 in mozilla::PresShell::ProcessReflowCommands(bool) src/layout/base/PresShell.cpp:9352:24
    #13 0x7f8129cdeeca in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4111:11
    #14 0x7f81274b2ced in FlushPendingNotifications src/obj-firefox/dist/include/mozilla/PresShell.h:1443:5
    #15 0x7f81274b2ced in FlushLayout src/dom/events/EventStateManager.cpp:5656:16
    #16 0x7f81274b2ced in mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*) src/dom/events/EventStateManager.cpp:691:7
    #17 0x7f8129d0c378 in mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*) src/layout/base/PresShell.cpp:7752:39
    #18 0x7f8129d02c6a in mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) src/layout/base/PresShell.cpp:7721:17
    #19 0x7f8129d09ba2 in mozilla::PresShell::EventHandler::HandleEventWithTarget(mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, bool, nsIContent**, nsIContent*) src/layout/base/PresShell.cpp:7608:17
    #20 0x7f81275e1dfe in HandleEventWithTarget src/obj-firefox/dist/include/mozilla/PresShell.h:650:25
    #21 0x7f81275e1dfe in mozilla::PointerEventHandler::DispatchPointerFromMouseOrTouch(mozilla::PresShell*, nsIFrame*, nsIContent*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*, nsIContent**) src/dom/events/PointerEventHandler.cpp:536:12
    #22 0x7f8129d067dd in mozilla::PresShell::EventHandler::DispatchPrecedingPointerEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsIContent*, bool, mozilla::PresShell::EventHandler::EventTargetData*, nsEventStatus*) src/layout/base/PresShell.cpp:6834:3
    #23 0x7f8129d01857 in mozilla::PresShell::EventHandler::HandleEventUsingCoordinates(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*, bool) src/layout/base/PresShell.cpp:6659:8
    #24 0x7f8129cff6ab in mozilla::PresShell::EventHandler::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) src/layout/base/PresShell.cpp:6485:12
    #25 0x7f8129cfe3ed in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) src/layout/base/PresShell.cpp:6411:23
    #26 0x7f812964270e in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) src/view/nsViewManager.cpp:751:18
    #27 0x7f81296420fd in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) src/view/nsView.cpp:1137:9
    #28 0x7f81296b2ead in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) src/widget/PuppetWidget.cpp:381:37
    #29 0x7f8123e99a0a in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) src/gfx/layers/apz/util/APZCCallbackHelper.cpp:544:21
    #30 0x7f8128d9bb2a in DispatchWidgetEventViaAPZ src/dom/ipc/BrowserChild.cpp:1804:10
    #31 0x7f8128d9bb2a in mozilla::dom::BrowserChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) src/dom/ipc/BrowserChild.cpp:1743:3
    #32 0x7f8128d9a9b2 in mozilla::dom::BrowserChild::ProcessPendingCoalescedMouseDataAndDispatchEvents() src/dom/ipc/BrowserChild.cpp:1595:7
    #33 0x7f8129c60be1 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1933:12
    #34 0x7f8129c70ab1 in TickDriver src/layout/base/nsRefreshDriver.cpp:373:13
    #35 0x7f8129c70ab1 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:350:7
    #36 0x7f8129c705db in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:367:5
    #37 0x7f8129c73c23 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:807:5
    #38 0x7f8129c73c23 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:727:16
    #39 0x7f8129c72f57 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) src/layout/base/nsRefreshDriver.cpp:622:9
    #40 0x7f812a5563e9 in mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&) src/layout/ipc/VsyncChild.cpp:65:16
    #41 0x7f812282d8cf in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:187:54
    #42 0x7f812229954e in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5876:32
    #43 0x7f8121b1a956 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2208:25
    #44 0x7f8121b15971 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2130:9
    #45 0x7f8121b17ee1 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1972:3
    #46 0x7f8121b18da7 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:2003:13
    #47 0x7f81209073da in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1250:14
    #48 0x7f812090e881 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #49 0x7f8121b23b0f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
    #50 0x7f8121a2d882 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #51 0x7f8121a2d882 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #52 0x7f8121a2d882 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #53 0x7f81296e9f08 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
    #54 0x7f812d7469a6 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
    #55 0x7f8121a2d882 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #56 0x7f8121a2d882 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #57 0x7f8121a2d882 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #58 0x7f812d7461f4 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
    #59 0x55ed71eefbec in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #60 0x55ed71eefbec in main src/browser/app/nsBrowserApp.cpp:272:18
Flags: in-testsuite?
Flags: needinfo?(cam)
Regressed by: 371787
Keywords: regression

[Tracking Requested - why for this release]: Smells like sec-high / sec-crit

A Pernosco session is available here: https://pernos.co/debug/nNxjpAjcHS8b7WgLc0ttwA/index.html

So I took a look, and this seems like an impedance mismatch between the PropertyProvider code and the rest of the SVG frame code. The PropertyProvider uses content offsets, and assumes the length of the string is the text fragment length. However the SVG code pokes at text runs, which can have merged text frames.

One option is disabling shaping across text frames in SVG... But I wonder, how is this supposed to work in regular CSS?

Flags: needinfo?(jfkthame)

I'm not actually sure what the correct behavior is per CSS. Things like

<div style="font: 100px serif; letter-spacing: 10px;">a<span style="letter-spacing: 100px;">&#x308;</span>.</div>

render differently in Firefox and Chrome.

Emilio you are right that the crash is because the PropertyProvider doesn't have data beyond the end of the frame, yet the call to ClusterRange produces a range that goes beyond the end. I think SVGTextFrame::DetermineCharPositions just needs to be more careful of such cases.

Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)

I think I've fixed DetermineCharPositions, but CharIterator::GetGlyphPartialAdvance and GetGlyphAdvance also will crash on content like this, since it will potentially want spacing information from adjacent text frames. I'll have to look at that aspect tomorrow, but for now I will land a patch to disable spacing in SVG text.

Keywords: leave-open

(In reply to Cameron McCormack (:heycam) from comment #4)

I'm not actually sure what the correct behavior is per CSS. Things like

<div style="font: 100px serif; letter-spacing: 10px;">a<span style="letter-spacing: 100px;">&#x308;</span>.</div>

render differently in Firefox and Chrome.

FWIW, I believe Chrome is wrong in that example: the sequence a&#x308; forms a single grapheme cluster 'ä', and letter-spacing should not be inserted inside it. See https://drafts.csswg.org/css-text-3/#letter-spacing-property and its use of typographic character unit (not simply "character") to describe the behavior.

Flags: needinfo?(jfkthame)

Note that Firefox's behavior is also wrong per spec, as there should be only 10px letter-spacing between the ä and the period, but that's a separate issue. AFAIR, no browser currently implements letter-spacing across element boundaries in the way the spec describes.

Attachment #9111841 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Adding rating as per comment 1

Keywords: sec-high

Can we land a crashtest for this?

Group: layout-core-security → core-security-release
Flags: needinfo?(cam)

Yes. I'll land it as part of the followup work in bug 1600855.

Flags: needinfo?(cam)
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

The test case (comment 0) crashed using an affected asan build (Nightly from 2019-11-27).

The issue is verified as fixed on the latest asan Beta build 72.0b12 (20191230025642), under Ubuntu 18.04 x64.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Crash Signature: [@ CharAt] [@ IsCSSWordSpacingSpace] [@ nsTextFrame::PropertyProvider::GetSpacingInternal]
Group: core-security-release
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: